fwiw, I opened awxkee/yuvutils-rs#3

fwiw, I opened awxkee/yuvutils-rs#3

Thank you for following up on this. I'll watch the issue.

Nice work!! Do you have some numbers from criterion? How much the performance was improved?

Well, criterion is huge time saver depending on your HW, but it still uses same amount of compute:
rfx_enc time: [6.4386 ms 6.4813 ms 6.5296 ms]
change: [-85.105% -85.013% -84.895%] (p = 0.00 < 0.05)
Performance has improved.

suggestion: Put the benchmarks in a separate crate ironrdp-benchmarks or benchmarks.

what's the rationale to put tests and benchmark in various crates? We try to ensure a common framework for the various crates that way?

Nice work!! Do you have some numbers from criterion? How much the performance was improved?

Well, criterion is huge time saver depending on your HW, but it still uses same amount of compute: rfx_enc time: [6.4386 ms 6.4813 ms 6.5296 ms] change: [-85.105% -85.013% -84.895%] (p = 0.00 < 0.05) Performance has improved.

Good numbers! By "criterion" here, you mean "rayon" for parallelizing the encoding?

suggestion: Put the benchmarks in a separate crate ironrdp-benchmarks or benchmarks.

what's the rationale to put tests and benchmark in various crates? We try to ensure a common framework for the various crates that way?

For the tests, the rationale is explained at several places:

• ARCHITECTURE.md: https://github.com/Devolutions/IronRDP/blob/master/ARCHITECTURE.md#testing
• tests/main.rs:

  IronRDP/crates/ironrdp-testsuite-core/tests/main.rs

  Lines 3 to 12 in 58f31b8

  	//! Integration Tests (IT)
  	//!
  	//! Integration tests are all contained in this single crate, and organized in modules.
  	//! This is to prevent `rustc` to re-link the library crates with each of the integration
  	//! tests (one for each *.rs file / test crate under the `tests/` folder).
  	//! Performance implication: https://github.com/rust-lang/cargo/pull/5022#issuecomment-364691154
  	//!
  	//! This is also good for execution performance.
  	//! Cargo will run all tests from a single binary in parallel, but
  	//! binaries themselves are run sequentally.

EDIT: Also a recommend read with similar points made: https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html

I'll also add less objective opinions and personal tastes:

• The lint unused_crate_dependencies will detect less false-positives.
• We can put code useful across tests along in the library part of the crate, for instance the macros: https://github.com/Devolutions/IronRDP/blob/58f31b88e06209a0cf24bb17213ae081438e3e57/crates/ironrdp-testsuite-core/src/macros.rs
• The Cargo.toml is kept free from test-only elements which are not relevant for a consumer point of view (although I think it's typically fine)

For the benchmarks, in addition to the above arguments:

• It's easier for someone not super familiar with the codebase to discover about the benchmarks
• It's possible to see the list of all the benchmarks in the Cargo.toml
• As for the tooling, you have the autocompletion with cargo bench --bench ^<TAB> which also returns you the list of targets. (Assuming you have completers for that in your shell.)

We also have a single ironrdp-fuzz crate in the fuzz folder at the root of the workspace for similar reasons. It's also easier to have a single README.md documenting things at one easy to discover place.

That's about it for the rationale behind this suggestion.